Periodical external pathfinding scores merge#449
Periodical external pathfinding scores merge#449joostjager merged 3 commits intolightningdevkit:mainfrom
Conversation
e0b7070 to
58acc12
Compare
5858404 to
fdf2b5d
Compare
tnull
left a comment
There was a problem hiding this comment.
Thanks! Did a first pass, already looks pretty good!
5a42ff9 to
0cf5774
Compare
619c9c9 to
6ded8af
Compare
7bf68af to
14167eb
Compare
| async fn sync_external_scores( | ||
| logger: &Logger, scorer: &Mutex<CombinedScorer<Arc<Graph>, Arc<Logger>>>, | ||
| node_metrics: &RwLock<NodeMetrics>, kv_store: Arc<DynStore>, url: &String, | ||
| ) -> () { |
There was a problem hiding this comment.
Should this return a Result?
There was a problem hiding this comment.
I don't think so - this function cannot fail?
src/scoring.rs
Outdated
| match body { | ||
| Err(e) => { | ||
| log_error!(logger, "Failed to read external scores update: {}", e); | ||
| return; |
There was a problem hiding this comment.
Let's avoid these explicit returns if possible.
There was a problem hiding this comment.
Indeed, not necessary. I wanted to exit the parent function with these returns, but just letting it run would have the same result.
In this current revision I now have a few real ones, after unwrapping the matches a bit.
14167eb to
1a4b051
Compare
|
Comments addressed. Now need to decide in lightningdevkit/rust-lightning#3562 how to read the combined scorer from disk (with local and merged data), and apply that in this PR. |
7c9309d to
d6bb7cf
Compare
d6bb7cf to
5ae6a35
Compare
| ) -> () { | ||
| let response = tokio::time::timeout( | ||
| Duration::from_secs(EXTERNAL_PATHFINDING_SCORES_SYNC_TIMEOUT_SECS), | ||
| reqwest::get(url), |
There was a problem hiding this comment.
should create a reqwest client instead of doing reqwest::get, otherwise it needs to redo TLS and everything each time
There was a problem hiding this comment.
should create a reqwest client instead of doing
reqwest::get, otherwise it needs to redo TLS and everything each time
Right, but establishing a new connection once an hour is reasonable? And probably preferable compared to keep a connection open if we only use it once an hour?
|
This has been parked, but I suppose with the new |
Yes, please rebase! |
|
Gentle ping @joostjager |
5ae6a35 to
19bf276
Compare
|
Rebased |
tnull
left a comment
There was a problem hiding this comment.
Two minor comments, otherwise should look good.
0dfafb6 to
961e67f
Compare
|
@joostjager Feel free to squash the fixup. More detailed/verbose commit messages would also be appreciated. |
Save external pathfinding scores in a cache so that they will be available immediately after a node restart. Otherwise there might be a time window where new scores need to be downloaded still and the node operates on local data only.
961e67f to
52705d3
Compare
|
Squashed without changes. |
Fixes #313
Depends on lightningdevkit/rust-lightning#3562
Export functionality in #458